Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate /node/full/{path} routes #612

Merged
merged 6 commits into from
Nov 29, 2023

Conversation

danielballan
Copy link
Member

Until #478, the word "node" in tiled has two meanings:

  1. every node the in the logical "tree" of tiled's data model
  2. specifically, a container-like structure

In #478, we introduced the term container for (2). Now, every object in tiled in a "node", and every node has a "structure family" which may be container or table or array or ...

The routes GET /node/full/{path} and PUT /node/full/{path} are vestigial, from this legacy. They operate on both container and table structures, which have similar semantics, in that they can both be filtered by fields/columns.

This PR leaves the routes in place, for full backward-compatibility, but adds new routes:

GET /table/full/{path}
PUT /table/full/{path}
GET /container/full/{path}

(The omission of PUT /container/full/{path} is intentional. Putting container data has never been supported, but is planned.)

It updates the Python client to use these new routes.

The goal of this change is:

  • Reduce potential confusion about the meaning of "node"
  • Provide a memorable URLs: /table/full/... for getting a table is more natural than /node/full/....
  • Lay track for future divergence of the query parameters support for containers and tables. The semantics are broadly similar, but I can easily imagine growing support for parameters than only apply to one or the other.

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had imagined that the python client relied on the generic node/full/{path} route for implementing __getitem__(). On inspection, it appears that it instead uses the self.links attribute to find the correct URL path.

Is that correct?

README.md Show resolved Hide resolved
docs/source/explanations/compression.md Show resolved Hide resolved
docs/source/tutorials/plotly-integration.md Show resolved Hide resolved
tiled/client/dataframe.py Show resolved Hide resolved
tiled/server/router.py Outdated Show resolved Hide resolved
tiled/server/router.py Outdated Show resolved Hide resolved
tiled/server/router.py Outdated Show resolved Hide resolved
docs/source/reference/http-api-overview.md Outdated Show resolved Hide resolved
docs/source/reference/http-api-overview.md Show resolved Hide resolved
danielballan and others added 5 commits November 29, 2023 13:21
Co-authored-by: Padraic Shafer <[email protected]>
Co-authored-by: Padraic Shafer <[email protected]>
Co-authored-by: Padraic Shafer <[email protected]>
Co-authored-by: Padraic Shafer <[email protected]>
@danielballan
Copy link
Member Author

Yes, correct. To elaborate, /node/full (and now /container/full) is for downloading all the data+metadata below a given point in the logical tree as a big file---some format like HDF5 or JSON that can represent nested structures.

The __getitem__ method is using JSON descriptions from the /search and or /metadata routes, and it obtains the URLs from self.links.

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making these clarifying changes to the HTTP API.

@padraic-shafer padraic-shafer merged commit a778e66 into bluesky:main Nov 29, 2023
8 checks passed
@danielballan danielballan deleted the deprecate-node-full-routes branch November 29, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants